Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fixup DicomAnonymiser CTP implementation #2053

Merged
merged 56 commits into from
Jan 8, 2025
Merged

fixup DicomAnonymiser CTP implementation #2053

merged 56 commits into from
Jan 8, 2025

Conversation

rkm
Copy link
Member

@rkm rkm commented Dec 17, 2024 •
edited
Loading

Proposed Changes

This PR finishes the wrapper for ctp-anon-cli and adds tests.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New Feature (non-breaking change which adds functionality)
  • Breaking Change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation-Only Update (if none of the other choices apply)
    • In this case, ensure that the message of the head commit from the source branch is prefixed with [skip ci]

Checklist

By opening this PR, I confirm that I have:

  • Reviewed the contributing guidelines for this repository
  • Ensured that the PR branch is in sync with the target branch (i.e. it is automatically merge-able)
  • Updated any relevant API documentation
  • Created or updated any tests if relevant
  • Created a news file
    • NOTE: This must include any changes to any of the following files: default.yaml, any of the RabbitMQ server configurations, GlobalOptions.cs
  • Listed myself in the CONTRIBUTORS file 🚀
  • Requested a review by the @SMI/Reviewers team

Issues

@rkm
Copy link
Member Author

rkm commented Dec 18, 2024

CC @jas88 feel free to pick this up if you have time

@rkm rkm force-pushed the feature/dicom-anonymiser-2 branch from e411aaf to 66b49dd Compare January 6, 2025 11:34
rkm and others added 20 commits January 6, 2025 12:15
@rkm
fix namespace block
abc8cec
@rkm
add script to download ctp jar
8b6b3c5
@rkm
rename file
1a9ed44
@rkm
fix namespace block
0222012
@rkm
add SlnDirectoryInfo helper
c07b304
@rkm
add DisposableTempDir helper
f134d03
@rkm
enforce correct namespaces and check in build
bd1274b
@rkm
add basic test for ctp anon wrapper
28c86cb
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
584a3d4 
for more information, see https://pre-commit.ci
@rkm
download ctp-anon-cli jar in CI
b5f7376
@rkm
don't download if already exists
9eb54cb
@rkm
restore IFileSystem
2c1e6ed
@rkm
fix error handling in DicomAnonymiserConsumer
864d7ee
@rkm
re-enable & fix DicomAnonymiserConsumer tests
063a7d6
@rkm
fix types
cb33bff
@rkm
test downloading CTP jar for Windows as well
e5aa325
@rkm
delete old readme
4e3fde4
@rkm
ignore exceptions when deleting tempdir in CI
c19152c
@rkm
print stderr when CreateProcess fails
94debda
@rkm
fixup test
b500a43
@codecov Codecov
Copy link

codecov bot commented Jan 7, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 73.11828% with 25 lines in your changes missing coverage. Please review.

Project coverage is 66.68%. Comparing base (3cc4bfb) to head (637f1cb).
Report is 56 commits behind head on main.

Files with missing lines Patch % Lines
...es/DicomAnonymiser/Anonymisers/SmiCtpAnonymiser.cs 76.36% 8 Missing and 5 partials ⚠️
...s/DicomAnonymiser/Anonymisers/DefaultAnonymiser.cs 0.00% 9 Missing ⚠️
...services/DicomAnonymiser/Helpers/ProcessWrapper.cs 82.35% 2 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2053      +/-   ##
==========================================
+ Coverage   66.03%   66.68%   +0.64%     
==========================================
  Files         182      184       +2     
  Lines        6519     6535      +16     
  Branches      958      959       +1     
==========================================
+ Hits         4305     4358      +53     
+ Misses       1920     1877      -43     
- Partials      294      300       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rkm
Copy link
Member Author

rkm commented Jan 7, 2025

@jas88 any chance you could look at the failing windows tests? I think I'm missing something obvious!

@rkm rkm marked this pull request as ready for review January 8, 2025 10:15
@rkm rkm requested a review from jas88 January 8, 2025 15:15
@rkm
Copy link
Member Author

rkm commented Jan 8, 2025

Thanks @jas88! Are you happy for this to be merged now?

jas88
jas88 approved these changes Jan 8, 2025
View reviewed changes
Copy link
Member

@jas88 jas88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as long as the retained event handler is intentional.

@rkm rkm merged commit 32d800c into main Jan 8, 2025
19 checks passed
@rkm rkm deleted the feature/dicom-anonymiser-2 branch January 8, 2025 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jas88 jas88 jas88 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rkm @jas88